Skip to content

Fix broken retry logic when synchroniser is disconnected #1035

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
fayi-da merged 2 commits into from
Jun 10, 2025
Merged

Fix broken retry logic when synchroniser is disconnected #1035

fayi-da merged 2 commits into from
Jun 10, 2025

Conversation

@fayi-da
Copy link
Contributor

@fayi-da fayi-da commented Jun 6, 2025

Making the type more specific and change DOMAIN -> SYNCHRONIZER

Pull Request Checklist

Cluster Testing

  • If a cluster test is required, comment /cluster_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.
  • If a hard-migration test is required (from the latest release), comment /hdm_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.

PR Guidelines

  • Include any change that might be observable by our partners or affect their deployment in the release notes.
  • Specify fixed issues with Fixes #n, and mention issues worked on using #n
  • Include a screenshot for frontend-related PRs - see README or use your favorite screenshot tool

Merge Guidelines

  • Make the git commit message look sensible when squash-merging on GitHub (most likely: just copy your PR description).

fayi-da
fayi-da commented Jun 6, 2025
View reviewed changes
apps/splitwell/frontend/src/App.tsx Outdated
Copy link
Contributor Author

@fayi-da fayi-da Jun 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the crux of this change. The rest is cleanup + more specific types

Copy link
Contributor

@martinflorian-da martinflorian-da Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:D Took me a while to see (DOMAIN->SYNCHRONIZER)... Good catch!

@fayi-da fayi-da force-pushed the fayi/json-api-type-fix branch from 2578b1d to 31b4859 Compare June 6, 2025 16:59
@martinflorian-da
Copy link
Contributor

martinflorian-da commented Jun 10, 2025

@fayi-da Is the failing CI check related to your PR?

martinflorian-da
martinflorian-da approved these changes Jun 10, 2025
View reviewed changes
Copy link
Contributor

@martinflorian-da martinflorian-da left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

apps/splitwell/frontend/src/App.tsx Outdated
Copy link
Contributor

@martinflorian-da martinflorian-da Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:D Took me a while to see (DOMAIN->SYNCHRONIZER)... Good catch!

apps/common/frontend/src/utils/helpers.ts Outdated
const isDomainConnectionError = keywords.some(k => errResponse.body?.error?.includes(k));

return isDomainConnectionError && failureCount < 10;
return keywords.some(k => errResponse.body?.error?.code.includes(k));
Copy link
Contributor

@martinflorian-da martinflorian-da Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you're also making the check more specific, by checking only .code instead of the whole error. I don't know the data format here in detail so would prefer to keep the proven more general scope; but I guess you know what you're doing so fine with me.

Copy link
Contributor Author

@fayi-da fayi-da Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so would prefer to keep the proven more general scope; but I guess you know what you're doing so fine with me.

I actually prefer this too and since you feel the same, i'll revert it.

@fayi-da
Fix broken retry logic [ci]
aa01391 
Making the type more specific and change DOMAIN -> SYNCHRONIZER

Signed-off-by: fayi-da <[email protected]>
@fayi-da fayi-da force-pushed the fayi/json-api-type-fix branch 2 times, most recently from 2aae373 to 8faf946 Compare June 10, 2025 09:33
@fayi-da
Cleanup [ci]
f0768ab 
Signed-off-by: fayi-da <[email protected]>
@fayi-da fayi-da force-pushed the fayi/json-api-type-fix branch from 8faf946 to f0768ab Compare June 10, 2025 10:34
@fayi-da fayi-da enabled auto-merge (squash) June 10, 2025 10:41
@fayi-da fayi-da merged commit 1ca1cb7 into main Jun 10, 2025
60 checks passed
@fayi-da fayi-da deleted the fayi/json-api-type-fix branch June 10, 2025 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martinflorian-da martinflorian-da martinflorian-da approved these changes

@ray-roestenburg-da ray-roestenburg-da Awaiting requested review from ray-roestenburg-da

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fayi-da @martinflorian-da